Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MAYA-123013 obey file-format choice in layer editor #2334

Merged
merged 4 commits into from
May 13, 2022

Conversation

pierrebai-adsk
Copy link
Collaborator

When re-saving a modified layer, follow the current ASCII / Binary format selected in the layer editor.

Notes:

  • USD SdfLayer does not have a Save() function taking the desired format. The only option we have is to use Export().
  • The choice of binary / ASCII is in a sub-menu and is global to all layers and all stages. That may not be convenient for the user: changing it will affect all stages and all layers. Beware!

When re-saving a modified layer, follow the current ASCII / Binary format selected in the layer editor.

Notes:
- USD SdfLayer does not have a Save() function taking the desired format. The only option we have is to use Export().
- The choice of binary / ASCII is in a sub-menu and is global to all layers and all stages. That may not be convenient for the user: changing it will affect all stages and all layers. Beware!
layer()->Save();
PXR_NS::SdfFileFormat::FileFormatArguments args;
args["format"] = MayaUsd::utils::usdFormatArgOption();
layer()->Export(layer()->GetRealPath(), "", args);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems okay to me, but I'm wondering what are the implications of calling Export() vs Save()? @ppt-adsk Do you have any comments about this?

Copy link
Collaborator Author

@pierrebai-adsk pierrebai-adsk May 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use Export in many other places instead of Save when we want to control the output format.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per the docs, Save() saves to the previously-remembered file name, and fails if there is none. Export() saves a copy of the layer, and does not affect the previously-remembered file name. There is one important point: Save() will clear the layer's dirty state (i.e. SdfLayer::IsDirty() will return false), whereas Export() will not, so this change is introducing an important difference.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Export() calls _WriteToFile() which calls _MarkCurrentStateAsClean() if the filename did not change, which is our case since we're exporting to the same file (we pass GetRelPath()).

The differences I can see are:

  • Save() does nothing if the layer is not dirty. We could add a call to IsDirty().
  • Set modification time, which I don't think we care about.

Things that look different but are the same in the end:

  • Save() check IsMuted() and IsAnonymous() directly, but Export() ends-up doing the same check because _WriteToFile() calls PermissionToSave(0 when the filename is the same and that does the same checks.
  • Save(0 clears hints, but the hints are not used once the dirty state is reset.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... unfortunately, there are two other places in the code where we call Save() directly on the layer... so they will not be aware of the changed file format. I'm taking a deeper look but we will either have to modify those too or abandon the idea of switching file formats for already-saved layers.

Avoid calling Save() and always use Export() so that the output file format respect the one selected by the user. Provide a function to enforce that and avoid code duplication.
ppt-adsk
ppt-adsk previously approved these changes May 12, 2022
@pierrebai-adsk pierrebai-adsk added the ready-for-merge Development process is finished, PR is ready for merge label May 12, 2022
@pierrebai-adsk pierrebai-adsk removed the ready-for-merge Development process is finished, PR is ready for merge label May 12, 2022
Verify if the file name and format are the same as the ones kept in the layer. If they both match, then call Save() instead of Export(). For this to work, the initial anonymous layer needs to be created with the correct format.
- Forcing the format when creating anonynous layer in memory prevented loading from a different format.
- This happened in one test.
- Don't affect maya reference updater not AL command for teh same reasons.
@pierrebai-adsk pierrebai-adsk added the ready-for-merge Development process is finished, PR is ready for merge label May 13, 2022
Copy link
Collaborator

@seando-adsk seando-adsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, thanks.

@seando-adsk seando-adsk added the core Related to core library label May 13, 2022
@seando-adsk seando-adsk merged commit d4e08ae into dev May 13, 2022
@seando-adsk seando-adsk deleted the t_bailp/MAYA-123013/switch-ascii-binary branch May 13, 2022 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Related to core library ready-for-merge Development process is finished, PR is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants